Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix recent regressions not allowing Metals to pass its tests #2363

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Jul 4, 2024

Previously, if --best-effort option was set, every project would be compiled in the best effort mode, including those using scala 2. This first commit fixes the CodeLensLspSuite test. DebugProtocolSuite might have been broken by 7154823 (before that merge it works), but I am still investigating that (or maybe the broken metals test should change?)

@tgodzik
Copy link
Contributor

tgodzik commented Jul 4, 2024

I will look into the DAP changes and if need to adjust anything. Looks the CI here is failing?

jchyb added 2 commits July 4, 2024 13:21
Previously, if `--best-effort` option was set, every project would be
compiled in the best effort mode, including those using scala 2.
@jchyb jchyb force-pushed the fix-metals-issues branch from b9fab8c to 5c804c4 Compare July 4, 2024 11:22
@@ -337,7 +337,7 @@ object TestTask {
taskDefs.map {
case TaskDefWithFramework(taskDef, _) =>
selectedTests.get(taskDef.fullyQualifiedName()) match {
case None =>
case None | Some(Nil) =>
Copy link
Contributor Author

@jchyb jchyb Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the DebugProtocolSuite tests (previously it was

 selectedTests.get(taskDef.fullyQualifiedName()).getOrElse(Nil) match {
              case Nil =>
              case selectedTests =>

and I think the Some(Nil) got lost in the refactor)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ach! Good catch!

@jchyb
Copy link
Contributor Author

jchyb commented Jul 4, 2024

I removed some leftover logs, hopefully that should help with the CI tests here (no idea why they broke)

@jchyb jchyb changed the title WIP: Fix recent regressions not allowing Metals to pass its tests Fix recent regressions not allowing Metals to pass its tests Jul 4, 2024
@jchyb jchyb marked this pull request as ready for review July 4, 2024 12:01
@jchyb
Copy link
Contributor Author

jchyb commented Jul 4, 2024

Ready for review! This should fix all of the tests in metals from https://github.com/scalameta/metals/actions/runs/9760409386/job/26939422405?pr=5219 outside of MillServerCodeLensSuite in sbt integration (which was broken by my metals PR)

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! LGTM

@tgodzik tgodzik merged commit e757787 into scalacenter:main Jul 4, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants